feat: HMAC SHA256 Authentication #795

tswagger · 2023-08-23T22:39:12Z

Adding HMAC-SHA256 as an authenticator. This method has a slight security advantage to a standard token authentication by signing the request with a shared secret.

Usage and coding mirrors closely the established Access Token Authentication classes and methods.

References:

kenjis · 2023-08-24T00:28:34Z

Thank you for sending this PR!

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

kenjis · 2023-08-24T00:35:07Z

And we don't use git merge to update PR branches.
Please use git rebase instead.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

tswagger · 2023-08-24T00:35:07Z

Thank you for sending this PR!

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

@kenjis My apologies, I thought I had that setup correctly. Since I obviously didn't, how do I retroactively sign what I have submitted?

kenjis · 2023-08-24T00:42:01Z

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

tswagger · 2023-08-24T15:02:09Z

@kenjis I have rebased and signed my code. I appreciate your assistance on that.

kenjis · 2023-08-25T00:09:08Z

@tswagger Thank you!

tswagger · 2023-08-25T14:12:53Z

I am not sure how you would like me to address the final failed check. I intentionally mirrored the Authorize Tokens classes. I could create a shared trait or abstract parent class, but some of the differences, while subtle, are major enough to make that challenging.

kenjis · 2023-08-25T21:05:35Z

We will give it some consideration, so please leave it as it is.
In any case, this PR is too large to review easily.

Adding Trait or abstract classes could make the design worse.
We can also suppress errors by PHPCPD.

tswagger · 2023-08-25T21:15:23Z

That was my thought. I will leave it in your hands. Please let me know if you need anything else from me.

docs/guides/api_hmac_keys.md

src/Authentication/Authenticators/HMAC_SHA256.php

docs/authentication.md

docs/guides/api_hmac_keys.md

docs/authentication.md

docs/guides/api_hmac_keys.md

docs/authentication.md

Co-authored-by: John Paul E. Balandan, CPA <[email protected]>

Signed-off-by: tswagger <[email protected]>

Co-authored-by: kenjis <[email protected]>

Signed-off-by: tswagger <[email protected]>

Added AuthToken config as a separate config for Token/HMAC auth from JWT Updated test to reflect logging adjustment change. Signed-off-by: tswagger <[email protected]>

Co-authored-by: kenjis <[email protected]>

Signed-off-by: tswagger <[email protected]>

Co-authored-by: Pooya Parsa <[email protected]>

Signed-off-by: tswagger <[email protected]>

kenjis · 2023-09-18T23:58:26Z

Cannot reproduce the PHPStan errors.

(feature/HMAC $)$ vendor/bin/phpstan
Note: Using configuration file /Users/kenji/work/codeigniter/official/codeigniter-shield/phpstan.neon.dist.
 176/176 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors

 ------ -------------------------------------------------------------------- 
  Line   src/Models/UserModel.php                                            
 ------ -------------------------------------------------------------------- 
  215    Offset 'email' does not exist on array{username: string, status:    
         string, status_message: string, active: bool, last_active: string,  
         deleted_at: string}.                                                
  216    Cannot unset offset 'email' on array{username: string, status:      
         string, status_message: string, active: bool, last_active: string,  
         deleted_at: string}.                                                
  217    Offset 'password_hash' does not exist on array{username: string,    
         status: string, status_message: string, active: bool, last_active:  
         string, deleted_at: string}.                                        
  218    Cannot unset offset 'password_hash' on array{username: string,      
         status: string, status_message: string, active: bool, last_active:  
         string, deleted_at: string}.                                        
 ------ -------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------- 
  Line   tests/Unit/UserTest.php                                            
 ------ ------------------------------------------------------------------- 
         Ignored error pattern #^Cannot access property \$active on         
         array\|object\.$# in path                                          
         /home/runner/work/shield/shield/tests/Unit/UserTest.php was not    
         matched in reported errors.                                        
         Ignored error pattern #^Cannot access property \$password_hash on  
         array\|object\.$# in path                                          
         /home/runner/work/shield/shield/tests/Unit/UserTest.php was not    
         matched in reported errors.                                        
 ------ ------------------------------------------------------------------- 

Error:  [ERROR] Found 6 errors

https://github.com/codeigniter4/shield/actions/runs/6223741069/job/16907463318?pr=795

kenjis

LGTM!

kenjis · 2023-09-19T00:09:30Z

@tswagger Thank you!

kenjis · 2023-09-19T00:17:53Z

Oh, my dependencies were old.

  - Upgrading codeigniter/coding-standard (v1.7.8 => v1.7.9)
  - Upgrading codeigniter/phpstan-codeigniter (v1.2.0.70400 => v1.3.0.70400)
  - Upgrading nexusphp/cs-config (v3.15.0 => v3.16.0)
  - Upgrading phpstan/phpdoc-parser (1.24.0 => 1.24.1)

Fixed by #840

tswagger changed the title ~~HMAC SHA256~~ feat: HMAC SHA256 Authentication Aug 23, 2023

kenjis added GPG-Signing needed Pull requests that need GPG-Signing enhancement New feature or request labels Aug 24, 2023

tswagger force-pushed the feature/HMAC branch from d9ab070 to 33a07c9 Compare August 24, 2023 14:42

tswagger force-pushed the feature/HMAC branch from df13005 to 96083ea Compare August 24, 2023 22:37

kenjis removed the GPG-Signing needed Pull requests that need GPG-Signing label Aug 25, 2023

datamweb reviewed Aug 26, 2023

View reviewed changes

docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved

datamweb reviewed Aug 26, 2023

View reviewed changes

docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved

tswagger force-pushed the feature/HMAC branch from 0d1c979 to 54f2864 Compare August 30, 2023 19:20

kenjis reviewed Aug 31, 2023

View reviewed changes

src/Authentication/Authenticators/HMAC_SHA256.php Outdated Show resolved Hide resolved

kenjis reviewed Aug 31, 2023

View reviewed changes

docs/authentication.md Outdated Show resolved Hide resolved

kenjis reviewed Aug 31, 2023

View reviewed changes

docs/authentication.md Show resolved Hide resolved

paulbalandan reviewed Aug 31, 2023

View reviewed changes

datamweb closed this Sep 5, 2023

datamweb reopened this Sep 5, 2023